msgpack_ext: add string convertion for decimal datetime interval #491
Conversation
|
TestTarantoolBCDCompatibility is FAIL. |
|
return d.Decimal.String() - this method true, but slowly. I will correct the code and comments, at the moment I am looking for a solution to the problem. |
| return ptr.UnmarshalMsgpack(b) | ||
| } | ||
|
|
||
| // This method converts Datetime to String - formats to ISO8601. |
There was a problem hiding this comment.
The go-idiomatic way to comment a method.
| // This method converts Datetime to String - formats to ISO8601. | |
| // String converts Datetime to String - formats to ISO8601. |
| } | ||
|
|
||
| func TestDatetimeString(t *testing.T) { | ||
|
|
| exponent := d.Decimal.Exponent() | ||
|
|
||
| // If exponent is positive, then we use the standard method. | ||
| if exponent > 0 { | ||
| return d.Decimal.String() | ||
| } | ||
|
|
||
| scale := -exponent | ||
|
|
||
| if !coefficient.IsInt64() { | ||
| return d.Decimal.String() | ||
| } | ||
|
|
||
| int64Value := coefficient.Int64() | ||
|
|
||
| return d.stringFromInt64(int64Value, int(scale)) | ||
| } | ||
|
|
||
| // StringFromInt64 is an internal method for converting int64 | ||
| // and scale to a string (for numbers up to 19 digits) | ||
| func (d Decimal) stringFromInt64(value int64, scale int) string { | ||
|
|
||
| var buf [48]byte | ||
| pos := 0 | ||
|
|
||
| negative := value < 0 | ||
| if negative { | ||
| if value == math.MinInt64 { | ||
| return d.handleMinInt64(scale) | ||
| } | ||
| buf[pos] = '-' | ||
| pos++ | ||
| value = -value | ||
| } | ||
|
|
||
| str := strconv.FormatInt(value, 10) | ||
| length := len(str) | ||
|
|
||
| if scale == 0 { | ||
| if pos+length > len(buf) { | ||
| return d.Decimal.String() | ||
| } | ||
| copy(buf[pos:], str) | ||
| pos += length | ||
| return string(buf[:pos]) | ||
| } | ||
|
|
||
| if scale >= length { | ||
|
|
||
| required := 2 + (scale - length) + length | ||
| if pos+required > len(buf) { | ||
| return d.Decimal.String() | ||
| } | ||
|
|
||
| buf[pos] = '0' | ||
| buf[pos+1] = '.' | ||
| pos += 2 | ||
|
|
||
| zeros := scale - length | ||
| for i := 0; i < zeros; i++ { | ||
| buf[pos] = '0' | ||
| pos++ | ||
| } | ||
|
|
||
| copy(buf[pos:], str) | ||
| pos += length | ||
| } else { | ||
|
|
||
| integerLen := length - scale | ||
|
|
||
| required := integerLen + 1 + scale | ||
| if pos+required > len(buf) { | ||
| return d.Decimal.String() | ||
| } | ||
|
|
||
| copy(buf[pos:], str[:integerLen]) | ||
| pos += integerLen | ||
|
|
||
| buf[pos] = '.' | ||
| pos++ | ||
|
|
||
| copy(buf[pos:], str[integerLen:]) | ||
| pos += scale | ||
| } | ||
|
|
||
| return string(buf[:pos]) | ||
| } | ||
| func (d Decimal) handleMinInt64(scale int) string { | ||
| const minInt64Str = "9223372036854775808" | ||
|
|
||
| var buf [48]byte | ||
| pos := 0 | ||
|
|
||
| buf[pos] = '-' | ||
| pos++ | ||
|
|
||
| length := len(minInt64Str) | ||
|
|
||
| if scale == 0 { | ||
| if pos+length > len(buf) { | ||
| return "-" + minInt64Str // Fallback. | ||
| } | ||
| copy(buf[pos:], minInt64Str) | ||
| pos += length | ||
| return string(buf[:pos]) | ||
| } | ||
|
|
||
| if scale >= length { | ||
| required := 2 + (scale - length) + length | ||
| if pos+required > len(buf) { | ||
| // Fallback. | ||
| result := "0." + strings.Repeat("0", scale-length) + minInt64Str | ||
| return "-" + result | ||
| } | ||
|
|
||
| buf[pos] = '0' | ||
| buf[pos+1] = '.' | ||
| pos += 2 | ||
|
|
||
| zeros := scale - length | ||
| for i := 0; i < zeros; i++ { | ||
| buf[pos] = '0' | ||
| pos++ | ||
| } | ||
|
|
||
| copy(buf[pos:], minInt64Str) | ||
| pos += length | ||
| } else { | ||
| integerLen := length - scale | ||
| required := integerLen + 1 + scale | ||
| if pos+required > len(buf) { | ||
| return d.Decimal.String() // Fallback | ||
| } | ||
|
|
||
| copy(buf[pos:], minInt64Str[:integerLen]) | ||
| pos += integerLen | ||
|
|
||
| buf[pos] = '.' | ||
| pos++ | ||
|
|
||
| copy(buf[pos:], minInt64Str[integerLen:]) | ||
| pos += scale | ||
| } | ||
|
|
||
| return string(buf[:pos]) | ||
| } | ||
|
|
There was a problem hiding this comment.
This code works faster than if you call the method d.Decimal.String() . Is it okay to leave it?
There was a problem hiding this comment.
Please, rename it to String and add benchmarks to compare with shopspring.Decimal if so.
There was a problem hiding this comment.
In general, I found out what the problem was with launching benchmarks, I just had to launch it without Tarantool.
There was a problem hiding this comment.
According to the results, my conversion function is twice as good if we have a lot of examples of numbers int64. After int64, it is slower by 100 nanoseconds. Thus, we have a compromise: for small numbers (which fit into an int64) there is a gain, for large numbers there is a small loss.
But in real-world scenarios, small numbers are probably more common. It is also worth noting that the difference in performance for large numbers is not very big (866.4 ns/op vs. 788.5 ns/op),
while for small numbers, the gain was several times.
There was a problem hiding this comment.
I note that for numbers that do not fit into Int 64, the Decimal.String() function is also used, but not immediately, but after checking the number for the value, due to this, the stringOptimized function, which I will soon rename to just String(), works a little slower.
And there are extra allocations in the case of large numbers, d.Decimal.Efficient() - creates a copy of big.Int
d.Decimal.Exponent() is a simple field, but it's still a method call.
coefficient.IsInt64() - big size check.Int
For large numbers, these operations create additional allocations and take time, even when we know that a fallback is inevitable.
There was a problem hiding this comment.
Therefore, I will try to make a simpler check for the value of the number.
There was a problem hiding this comment.
TestRealTarantoolUsage is broken. We need to decide whether the String() method for Decimal should return insignificant zeros after . or not. In general, in Tarantula, it is expected that 100.00 will be like 100, but the BCDCompability test expects the opposite. BCDCompability is not broken because of the String() method.
babyTsakhes
force-pushed
the
babyTsakhes/gh-322-add-string-convertion-for-decimal-datetime-interval
branch
from
b214fc6 to
a66d851
Compare
Added a benchmark, which shows that the code for decimal is optimized two or more times for string conversion than the code from the library. Added a datetime, Interval type conversion function to a string, added tests for this function. Added #322
babyTsakhes
force-pushed
the
babyTsakhes/gh-322-add-string-convertion-for-decimal-datetime-interval
branch
from
a66d851 to
679282c
Compare
msgpack_ext: add string() for decimal
Added a decimal type conversion function to a string, added tests for this function. Fixed name of function. Add test for decimal convertion. I tried to write an optimized version, but there is a problem with values when working with BCD, insignificant zeros after the decimal point are lost.
Added #322